[Logs UI] Alerting #62806
[Logs UI] Alerting #62806
Conversation
…f route handler contexts (like the alert executor)
Kerry350
added
v7.8.0
v8.0.0
Feature:Logs UI
|
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
|
@Zacqary I've added you as the reviewer for this, please let me know if that's not okay. The only big(ish) caveat is the alerts management screen, which will be addressed in a followup. |
x-pack/plugins/infra/public/components/alerting/logs/expression_editor/criterion.tsx
Show resolved
Hide resolved
| }} | ||
| /> | ||
| ) : ( | ||
| <EuiFieldText |
There was a problem hiding this comment.
How about we add some double quotes around the <EuiFieldText> to signify that it's a string value instead of a number?
There was a problem hiding this comment.
Hmm, I think this could potentially mislead / frustrate people. I understand the intention, but I can also see people entering error and then seeing "error" and thinking "huh, I haven't added quotations".
Or, if there's a scenario where people actually want to have the quotations as part of the value they're matching, we either need to display double quotes which is messy, or not render them if the user has used quotes in the value. This again leads to a misleading situation.
I think it might be best to leave this one? Or defer to design for the next iteration?
There was a problem hiding this comment.
Okay we can leave this. Changing EQUALS to IS should be good enough.
| export async function registerMetricThresholdAlertType(alertingPlugin: PluginSetupContract) { | ||
| export async function registerMetricThresholdAlertType( | ||
| alertingPlugin: PluginSetupContract, | ||
| libs: InfraBackendLibs |
There was a problem hiding this comment.
libs is unused in this alert type, and on my end it doesn't look like we need to add this arg to get it to pass type check. Am I missing something?
There was a problem hiding this comment.
This was mostly laziness to deal with:
registerFns.forEach(fn => {
fn(alertingPlugin, libs);
});
it was easier just to pass them both the libs.
As the work Philip and I are doing has some overlap here, where we've both refactored the source configuration code and allowed access to libs in executors, and Philip has refactored this forEach line, I think it would be easier just to leave this for this merge. And deal with the small conflict that will arise in Philip's branch.
There was a problem hiding this comment.
Sounds good, we can do that.
|
@Zacqary Thanks for the review 👍 I've responded to your feedback, if you could take another look. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
First iteration of logs alerting
…bana into pipeline-editor-part-mvp-2 * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (152 commits) [Ingest pipelines] Simulate pipeline (elastic#64223) Ability to get scoped call cluster from alerting and action executors (elastic#64432) Add editApp and editPath to embeddable (elastic#64297) TSVB validation: Allow numeric values for axes (elastic#63553) [ML] Fixing optional plugin dependency types (elastic#64450) [Logs UI] Alerting (elastic#62806) [Endpoint] Show Policy Status on Host Details using Policy Response API (elastic#64116) [Maps] update LayerWizard previewLayer to take layerDescriptor instead of ISource (elastic#64461) Remove SO root property index signature (elastic#64434) [ML] Functional tests - stabilize job row details validations (elastic#64503) [Ingest] Add Global settings flyout (elastic#64276) Bump cypress dev-dependency from 4.2.0 to 4.4.1 (elastic#64408) Migrate saved object of type url to kibana platform (elastic#64043) [NP] Migrate ui capabilities (elastic#64185) Bump karma-mocha dev-dependency from 1.3.0 to 2.0.0 (elastic#64407) Migrate kql_telemetry saved object registration to Kibana platform (elastic#64149) Remove SO autocreateindex error and error page (elastic#64037) Fix issue with yarn.lock (elastic#64496) Bump @hapi/boom dependency from 7.4.2 to 7.4.11 (elastic#64433) Bump gonzales-pe dev-dependency from 4.2.4 to 4.3.0 (elastic#64401) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx # x-pack/plugins/ingest_pipelines/public/shared_imports.ts
Summary
This PR implements #61493. It adds the "core" of the first iteration of logs alerting. The end-to-end flow when adding an alert via the logs app should work.
Followups
Due to the size of this PR, and also working in parellel with other PRs, the following is to come in the next PR:
Alert management screen changes: there are caveats to editing an alert on the alerts management screen (i.e. the things you have access to via context). This flow will be addressed next, and should be considered broken with the work here. In part this is so that [Logs UI] Reimplement log source configuration routes in plain HTTP+JSON #64021 can be merged first, which addresses one of the issues there without having to create a workaround.
Tests. I would like to add executor tests at least.
The
occurtext mentioned here (after speaking with Felix this will appear after the "when log entries" line).Add a proper loading state to show when the fields data is loading (easier with the new logs source config hook).
Testing
SSL is necessary, which can be facilitated with:
yarn es snapshot --sslyarn start --sslAnd adding
ssl.verification_mode: nonein the Filebeat config.